Skip to content

Conversation

@devonh
Copy link
Member

@devonh devonh commented Oct 1, 2025

Implements sliding sync extension from: matrix-org/matrix-spec-proposals#4360
Companion endpoint added in #19041

Disclaimer: I used Claude Code in a consultative manner to assist with parts of this PR.
I used it mainly for considering options for the SQL query and to help generate test cases.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct (run the linters)

) -> JsonDict:
"""Get related events of a event, ordered by topological ordering.
TODO Accept a PaginationConfig instead of individual pagination parameters.
Copy link
Member Author

@devonh devonh Oct 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been done already, the comment just wasn't updated.

@devonh devonh marked this pull request as ready for review October 10, 2025 22:49
@devonh devonh requested a review from a team as a code owner October 10, 2025 22:49
Comment on lines 1153 to 1164
async def get_thread_updates_for_user(
self,
*,
user_id: str,
from_token: Optional[RoomStreamToken] = None,
to_token: Optional[RoomStreamToken] = None,
limit: int = 5,
include_thread_roots: bool = False,
) -> Tuple[Sequence[ThreadUpdateInfo], Optional[StreamToken]]:
"""Get a list of updated threads, ordered by stream ordering of their
latest reply, filtered to only include threads in rooms where the user
is currently joined.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like this should be taking in the list of rooms that are being returned in the Sliding Sync response and only returning updates for those.

Copy link
Member Author

@devonh devonh Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about that, and even tried implementing it.
The problem becomes what should we return as a token that is usable on the /thread_updates endpoint?
I assume we would need to follow the same pattern as the receipts extension and handle initial, live, and previously rooms separately. The options for what token to return for pagination would be either the oldest from the three sets, or the newest from the three sets.
In the case of the oldest, further pagination could result in missing updates between the oldest token and the tokens from the other two sets.
In the case of the newest, further pagination could result in receiving duplicate entries.

Then there is the issue of how would you associate the set of rooms/lists with the token such that using it for pagination on /thread_updates makes sense. And would using the new endpoint update the sliding sync connection state? What if using different rooms/lists on the new endpoint than in sliding sync?

It's not that we couldn't come up with solutions to these things. I just felt they quickly over complicated things.
The overall intention was to just be able to track new events in threads easily and separately from the entire set of events to ensure clients can manage a threads notification center without having to paginate through all of a user's event history since the last time they logged in.

Maybe it's worth the complication to add room/list filtering, but I'm not convinced after having tried to implement it.

Copy link
Contributor

@MadLittleMods MadLittleMods Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could implement a bulk /messages endpoint, where the client would specify multiple rooms and prev_batch tokens.

-- matrix-org/matrix-spec-proposals#4186

Per the above, I think we would probably want to use all of the prev_batch tokens from all of the rooms timeline. Perhaps clunky, but maybe good.

And this also gets around the problem where you can't get to the thread updates that you care about without paginating through the bulk. This way, you can specify the rooms you want to get more thread updates in.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code now uses the sliding sync room lists to filter results.

Comment on lines +61 to +62
# Helper function to extract StreamToken from either StreamToken or SlidingSyncStreamToken format
def extract_stream_token(token_str: str) -> str:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this used? Where are we using a SlidingSyncStreamToken for another endpoint?

Just trying to understand the landscape as I think we eventually need this kind of thing (matrix-org/matrix-spec-proposals#4186 (comment))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It gets used in the /thread_updates companion endpiont #19041

I agree that the compatibility should be specced.
The spec mentions usage of tokens from /sync in the /messages api https://spec.matrix.org/v1.16/client-server-api/#get_matrixclientv3roomsroomidmessages
And the simplified sliding sync MSC says the prev_batch token is compatible with /messages
https://github.com/matrix-org/matrix-spec-proposals/blob/erikj/sss/proposals/4186-simplified-sliding-sync.md
prev_batch -> A token that can be passed as a start parameter to the /rooms/<room_id>/messages API to retrieve earlier messages.

So it seems like the landscape is already leaning in that direction, but maybe hasn't been formalized.

@@ -0,0 +1 @@
Add experimental support for MSC4360: Sliding Sync Threads Extension.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also comments on the MSC that may change how we approach things here -> matrix-org/matrix-spec-proposals#4360 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants